Skip to content

Conversation

@cj-vana
Copy link
Collaborator

@cj-vana cj-vana commented Oct 7, 2025

User description

Previously, moving items in the Run of Show would recalculate ALL start times based on cumulative duration, destroying carefully planned gaps between items (stage changes, intermissions, etc.).

Now implements smart gap-preserving logic:

  • Items with positive gaps (intentional buffers) → gap size preserved
  • Items with no gaps or overlaps → auto-adjusted to follow previous item
  • Items without start times → calculated automatically
  • Item numbers stay sequential after reordering

This allows users to maintain their production timing intentions while reordering the show flow.

Fixes issue where reordering would mess up all timings.

🤖 Generated with Claude Code


PR Type

Bug fix


Description

  • Preserve intentional gaps when reordering Run of Show items

  • Implement smart gap-preserving logic for timing calculations

  • Fix issue where reordering destroyed planned intermissions/breaks

  • Maintain sequential item numbering after reordering


Diagram Walkthrough

flowchart LR
  A["Original Items"] --> B["Swap Items"]
  B --> C["Calculate Gaps"]
  C --> D["Preserve Positive Gaps"]
  C --> E["Auto-adjust No Gaps"]
  D --> F["Updated Items"]
  E --> F
Loading

File Walkthrough

Relevant files
Bug fix
RunOfShowEditor.tsx
Implement gap-preserving reorder logic                                     

apps/web/src/pages/RunOfShowEditor.tsx

  • Replace simple cumulative time calculation with gap-preserving logic
  • Add gap detection between existing and expected start times
  • Preserve positive gaps (intentional buffers) when reordering
  • Auto-adjust items with no gaps or overlaps to follow previous item
+36/-8   

Previously, moving items in the Run of Show would recalculate ALL start
times based on cumulative duration, destroying carefully planned gaps
between items (stage changes, intermissions, etc.).

Now implements smart gap-preserving logic:
- Items with positive gaps (intentional buffers) → gap size preserved
- Items with no gaps or overlaps → auto-adjusted to follow previous item
- Items without start times → calculated automatically
- Item numbers stay sequential after reordering

This allows users to maintain their production timing intentions while
reordering the show flow.

Fixes issue where reordering would mess up all timings.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@netlify
Copy link

netlify bot commented Oct 7, 2025

Deploy Preview for sounddocsbeta ready!

Name Link
🔨 Latest commit 9f687a5
🔍 Latest deploy log https://app.netlify.com/projects/sounddocsbeta/deploys/68e54b94ec17cf0008df36c6
😎 Deploy Preview https://deploy-preview-108--sounddocsbeta.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Oct 7, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Oct 7, 2025

PR Code Suggestions ✨

Latest suggestions up to c44bafa

CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Fix numbering off-by-one bug

Fix an off-by-one error in item numbering logic. The current implementation
starts numbering at "2" instead of "1"; the suggestion corrects this and also
ensures headers have a consistent, non-conflicting number.

apps/web/src/pages/RunOfShowEditor.tsx [581-668]

 // Step 3: Recalculate item numbers and start times using stored gaps
 let itemCount = 1;
 let cumulativeEndTimeSeconds = firstAnchoredStart;
 
 const recalculatedItems = itemsWithGaps.map((item) => {
   if (item.type === "header") {
     const existingStartTime = parseTimeToSeconds(item.startTime || "");
     let newHeaderStart: string;
 
     if (existingStartTime !== null) {
-      // Advance pointer to header time if it's ahead, clamp if behind
       if (existingStartTime > cumulativeEndTimeSeconds) {
         cumulativeEndTimeSeconds = existingStartTime;
         newHeaderStart = item.startTime!;
       } else {
         newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds);
       }
     } else {
       newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds);
     }
 
-    // Return header without calculatedGap property
     return {
       id: item.id,
       type: item.type,
-      itemNumber: item.itemNumber,
+      itemNumber: "", // keep headers unnumbered to avoid conflicts
       startTime: newHeaderStart,
       highlightColor: item.highlightColor,
       headerTitle: item.headerTitle,
     } as RunOfShowItem;
   }
 
-  // Items - extract gap and other properties
   const itemWithGap = item as RunOfShowItem & { calculatedGap?: number };
   const gap = itemWithGap.calculatedGap || 0;
   const newStartTime = cumulativeEndTimeSeconds + gap;
   const durationSeconds = parseDurationToSeconds(item.duration || "");
 
-  itemCount++;
+  const currentItemNumber = itemCount.toString();
+  // advance pointers after assigning the current number
   cumulativeEndTimeSeconds = newStartTime;
   if (durationSeconds !== null) {
     cumulativeEndTimeSeconds += durationSeconds;
   }
+  itemCount++;
 
-  // Return item without calculatedGap property
   return {
     id: item.id,
     type: item.type,
-    itemNumber: itemCount.toString(),
+    itemNumber: currentItemNumber,
     startTime: formatSecondsToTime(newStartTime),
     highlightColor: item.highlightColor,
     preset: item.preset,
     duration: item.duration,
     privateNotes: item.privateNotes,
     productionNotes: item.productionNotes,
     audio: item.audio,
     video: item.video,
     lights: item.lights,
-    // Include custom column values
     ...Object.keys(item)
       .filter(
         (key) =>
           ![
             "id",
             "type",
             "itemNumber",
             "startTime",
             "highlightColor",
             "preset",
             "duration",
             "privateNotes",
             "productionNotes",
             "audio",
             "video",
             "lights",
             "calculatedGap",
             "headerTitle",
           ].includes(key),
       )
       .reduce(
         (acc, key) => {
           acc[key] = item[key as keyof RunOfShowItem];
           return acc;
         },
         {} as Record<string, string | number | boolean | undefined>,
       ),
   } as RunOfShowItem;
 });

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies an off-by-one bug in item re-numbering where itemCount is incremented before use, causing numbering to start at "2". The proposed fix resolves this functional error, which is a significant improvement to correctness.

Medium
Anchor start using items only

Modify the logic for finding the initial start time to only consider item types,
not header types. This prevents a header with an old start time from incorrectly
shifting the entire schedule backward.

apps/web/src/pages/RunOfShowEditor.tsx [572-579]

-// Establish initial cumulative time from earliest anchored start (header or item)
+// Establish initial cumulative time from earliest anchored start based on items only
 const firstAnchoredStart = (() => {
   for (const it of itemsWithGaps) {
+    if (it.type !== "item") continue;
     const t = parseTimeToSeconds(it.startTime || "");
     if (t !== null) return t;
   }
   return 0;
 })();
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that anchoring the start time to a header can cause unintended time shifts if the header's time is earlier than the first item's. By restricting the anchor search to items only, it ensures the schedule recalculation is based on the flow of actual content, improving the logic's correctness.

Medium
Guard against NaN and negative gaps

Add checks to prevent NaN values during gap calculation. The change ensures that
invalid time strings do not propagate NaN values, making the time calculations
more robust.

apps/web/src/pages/RunOfShowEditor.tsx [522-564]

-// Step 1: Calculate gaps BEFORE reordering
-// This captures the gap each item has relative to its IMMEDIATE previous item
 const itemsWithGaps = items.map((item, index) => {
   let gap = 0;
 
   if (item.type === "item") {
     const itemStartTime = parseTimeToSeconds(item.startTime || "");
-
-    if (itemStartTime === null) {
-      // Without a valid start time we cannot compute a reliable gap
+    if (itemStartTime === null || !Number.isFinite(itemStartTime)) {
       return { ...item, calculatedGap: 0 };
     }
 
-    // Walk backwards to find the previous item's end time, skipping headers
     let prevEndTime: number | null = null;
     for (let i = index - 1; i >= 0; i--) {
       const prev = items[i];
       if (prev.type !== "item") continue;
 
       const prevStart = parseTimeToSeconds(prev.startTime || "");
-      const prevDuration = parseDurationToSeconds(prev.duration || "");
+      const prevDurationParsed = parseDurationToSeconds(prev.duration || "");
+      const prevDurationSafe =
+        prevDurationParsed !== null && Number.isFinite(prevDurationParsed)
+          ? prevDurationParsed
+          : 0;
 
-      if (prevStart !== null) {
-        prevEndTime = prevStart + (prevDuration ?? 0);
+      if (prevStart !== null && Number.isFinite(prevStart)) {
+        const end = prevStart + prevDurationSafe;
+        prevEndTime = Number.isFinite(end) ? end : prevStart; // fallback to start if something goes wrong
         break;
       }
-
-      // If no start, but we have duration, keep looking further back
-      // However, don't add duration without a known anchor start.
-      if (prevStart === null) continue;
     }
 
     if (prevEndTime !== null) {
-      gap = itemStartTime - prevEndTime;
-      if (gap < 0) gap = 0;
+      const rawGap = itemStartTime - prevEndTime;
+      gap = Number.isFinite(rawGap) && rawGap > 0 ? rawGap : 0;
     } else {
-      // No previous item with a valid start; treat as no gap to preserve
       gap = 0;
     }
   }
 
   return { ...item, calculatedGap: gap };
 });
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out that the existing code does not handle potential NaN values from time parsing, which could lead to invalid calculations. While the original code already clamps negative gaps, adding Number.isFinite checks improves robustness against malformed time string inputs.

Low
Possible issue
Correct numbering and header anchoring

Fix an off-by-one error in item numbering where itemCount was incremented before
being assigned. Additionally, correct the logic for recalculating header start
times to prevent them from being pulled forward unintentionally.

apps/web/src/pages/RunOfShowEditor.tsx [585-668]

 const recalculatedItems = itemsWithGaps.map((item) => {
   if (item.type === "header") {
     const existingStartTime = parseTimeToSeconds(item.startTime || "");
     let newHeaderStart: string;
 
-    if (existingStartTime !== null) {
-      // Advance pointer to header time if it's ahead, clamp if behind
-      if (existingStartTime > cumulativeEndTimeSeconds) {
-        cumulativeEndTimeSeconds = existingStartTime;
-        newHeaderStart = item.startTime!;
-      } else {
-        newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds);
-      }
+    if (existingStartTime !== null && existingStartTime > cumulativeEndTimeSeconds) {
+      // Only advance pointer if header explicitly anchors to a future time
+      cumulativeEndTimeSeconds = existingStartTime;
+      newHeaderStart = item.startTime!;
     } else {
+      // Do not clamp backwards; keep header aligned to current pointer
       newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds);
     }
 
-    // Return header without calculatedGap property
     return {
       id: item.id,
       type: item.type,
-      itemNumber: item.itemNumber,
+      itemNumber: "", // headers should not consume or display item numbers here
       startTime: newHeaderStart,
       highlightColor: item.highlightColor,
       headerTitle: item.headerTitle,
     } as RunOfShowItem;
   }
 
   // Items - extract gap and other properties
   const itemWithGap = item as RunOfShowItem & { calculatedGap?: number };
   const gap = itemWithGap.calculatedGap || 0;
   const newStartTime = cumulativeEndTimeSeconds + gap;
   const durationSeconds = parseDurationToSeconds(item.duration || "");
 
+  // Set number before increment to avoid off-by-one
+  const currentItemNumber = itemCount.toString();
   itemCount++;
+
   cumulativeEndTimeSeconds = newStartTime;
   if (durationSeconds !== null) {
     cumulativeEndTimeSeconds += durationSeconds;
   }
 
-  // Return item without calculatedGap property
   return {
     id: item.id,
     type: item.type,
-    itemNumber: itemCount.toString(),
+    itemNumber: currentItemNumber,
     startTime: formatSecondsToTime(newStartTime),
     highlightColor: item.highlightColor,
     preset: item.preset,
     duration: item.duration,
     privateNotes: item.privateNotes,
     productionNotes: item.productionNotes,
     audio: item.audio,
     video: item.video,
     lights: item.lights,
-    // Include custom column values
     ...Object.keys(item)
       .filter(
         (key) =>
           ![
             "id",
             "type",
             "itemNumber",
             "startTime",
             "highlightColor",
             "preset",
             "duration",
             "privateNotes",
             "productionNotes",
             "audio",
             "video",
             "lights",
             "calculatedGap",
             "headerTitle",
           ].includes(key),
       )
       .reduce(
         (acc, key) => {
           acc[key] = item[key as keyof RunOfShowItem];
           return acc;
         },
         {} as Record<string, string | number | boolean | undefined>,
       ),
   } as RunOfShowItem;
 });

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies and fixes two distinct bugs: an off-by-one error in item numbering and incorrect logic for header time recalculation that could lead to unexpected time shifts.

Medium
General
Accumulate durations to compute gaps

Improve the gap calculation logic to correctly handle chains of items where some
intermediate items lack a startTime. The current implementation fails to
preserve gaps in this scenario, but the suggested change accumulates durations
to find a proper anchor and calculate the correct gap.

apps/web/src/pages/RunOfShowEditor.tsx [524-564]

 const itemsWithGaps = items.map((item, index) => {
   let gap = 0;
 
   if (item.type === "item") {
     const itemStartTime = parseTimeToSeconds(item.startTime || "");
 
     if (itemStartTime === null) {
       // Without a valid start time we cannot compute a reliable gap
       return { ...item, calculatedGap: 0 };
     }
 
-    // Walk backwards to find the previous item's end time, skipping headers
+    // Walk backwards to find an anchor start, accumulating durations
     let prevEndTime: number | null = null;
+    let accumulatedDurations = 0;
+
     for (let i = index - 1; i >= 0; i--) {
       const prev = items[i];
       if (prev.type !== "item") continue;
 
       const prevStart = parseTimeToSeconds(prev.startTime || "");
-      const prevDuration = parseDurationToSeconds(prev.duration || "");
+      const prevDuration = parseDurationToSeconds(prev.duration || "") ?? 0;
 
       if (prevStart !== null) {
-        prevEndTime = prevStart + (prevDuration ?? 0);
+        prevEndTime = prevStart + prevDuration + accumulatedDurations;
         break;
+      } else {
+        // No start; accumulate duration and continue searching for an earlier anchor
+        accumulatedDurations += prevDuration;
+        continue;
       }
-
-      // If no start, but we have duration, keep looking further back
-      // However, don't add duration without a known anchor start.
-      if (prevStart === null) continue;
     }
 
     if (prevEndTime !== null) {
       gap = itemStartTime - prevEndTime;
       if (gap < 0) gap = 0;
     } else {
-      // No previous item with a valid start; treat as no gap to preserve
       gap = 0;
     }
   }
 
   return { ...item, calculatedGap: gap };
 });
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a flaw in the gap calculation logic where gaps could be lost if an intermediate item lacks a start time. The proposed fix improves the robustness of the gap preservation feature.

Medium
  • Update

Previous suggestions

✅ Suggestions up to commit 256df11
CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Correct gap computation with headers
Suggestion Impact:The commit implements the suggested backward iteration skipping non-item headers, adds an early return when the current item's start time is null, computes prevEndTime using prevStart + duration, and sets gap based on that with non-negative clamp.

code diff:

+        if (itemStartTime === null) {
+          // Without a valid start time we cannot compute a reliable gap
+          return { ...item, calculatedGap: 0 };
+        }
+
+        // Walk backwards to find the previous item's end time, skipping headers
+        let prevEndTime: number | null = null;
+        for (let i = index - 1; i >= 0; i--) {
+          const prev = items[i];
+          if (prev.type !== "item") continue;
+
+          const prevStart = parseTimeToSeconds(prev.startTime || "");
+          const prevDuration = parseDurationToSeconds(prev.duration || "");
+
+          if (prevStart !== null) {
+            prevEndTime = prevStart + (prevDuration ?? 0);
+            break;
           }
 
-          // Gap is the difference between this item's start and when it "should" start
+          // If no start, but we have duration, keep looking further back
+          // However, don't add duration without a known anchor start.
+          if (prevStart === null) continue;
+        }
+
+        if (prevEndTime !== null) {
           gap = itemStartTime - prevEndTime;
-          // Only preserve positive gaps (intentional buffers)
           if (gap < 0) gap = 0;
+        } else {
+          // No previous item with a valid start; treat as no gap to preserve
+          gap = 0;
         }

Refactor the gap calculation logic to correctly and efficiently find the
previous item's end time by iterating backward and skipping headers. This fixes
a bug where gaps were miscalculated.

apps/web/src/pages/RunOfShowEditor.tsx [524-558]

 const itemsWithGaps = items.map((item, index) => {
   let gap = 0;
 
   if (item.type === "item") {
     const itemStartTime = parseTimeToSeconds(item.startTime || "");
+    if (itemStartTime === null) {
+      // Without a valid start time we cannot compute a reliable gap
+      return { ...item, calculatedGap: 0 };
+    }
 
-    if (itemStartTime !== null && index > 0) {
-      // Calculate cumulative end time of all previous items
-      let prevEndTime = 0;
-      for (let i = 0; i < index; i++) {
-        const prevItem = items[i];
-        if (prevItem.type === "item") {
-          const prevStart = parseTimeToSeconds(prevItem.startTime || "");
-          const prevDuration = parseDurationToSeconds(prevItem.duration || "");
+    // Walk backwards to find the previous item's end time, skipping headers
+    let prevEndTime: number | null = null;
+    for (let i = index - 1; i >= 0; i--) {
+      const prev = items[i];
+      if (prev.type !== "item") continue;
 
-          if (prevStart !== null) {
-            prevEndTime = prevStart;
-            if (prevDuration !== null) {
-              prevEndTime += prevDuration;
-            }
-          } else if (prevDuration !== null) {
-            prevEndTime += prevDuration;
-          }
-        }
+      const prevStart = parseTimeToSeconds(prev.startTime || "");
+      const prevDuration = parseDurationToSeconds(prev.duration || "");
+
+      if (prevStart !== null) {
+        prevEndTime = prevStart + (prevDuration ?? 0);
+        break;
       }
 
-      // Gap is the difference between this item's start and when it "should" start
+      // If no start, but we have duration, keep looking further back
+      // However, don't add duration without a known anchor start.
+      if (prevStart === null) continue;
+    }
+
+    if (prevEndTime !== null) {
       gap = itemStartTime - prevEndTime;
-      // Only preserve positive gaps (intentional buffers)
       if (gap < 0) gap = 0;
+    } else {
+      // No previous item with a valid start; treat as no gap to preserve
+      gap = 0;
     }
   }
 
   return { ...item, calculatedGap: gap };
 });

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a significant logical flaw and inefficiency in the PR's gap calculation logic and provides a robust, correct, and more efficient implementation.

High
Possible issue
Preserve absolute schedule offset
Suggestion Impact:The commit added computation of firstAnchoredStart from the first valid start time and set cumulativeEndTimeSeconds to that value, preserving schedule offset during recalculation. It also adjusted header handling accordingly.

code diff:

+    // Establish initial cumulative time from earliest anchored start (header or item)
+    const firstAnchoredStart = (() => {
+      for (const it of itemsWithGaps) {
+        const t = parseTimeToSeconds(it.startTime || "");
+        if (t !== null) return t;
+      }
+      return 0;
+    })();
+
     // Step 3: Recalculate item numbers and start times using stored gaps
     let itemCount = 1;
-    let cumulativeEndTimeSeconds = 0;
+    let cumulativeEndTimeSeconds = firstAnchoredStart;
 

Initialize cumulativeEndTimeSeconds based on the first item's start time instead
of hardcoding it to zero. This preserves any absolute schedule offset when
recalculating times after moving an item.

apps/web/src/pages/RunOfShowEditor.tsx [560-608]

 // Step 2: Swap items
 [itemsWithGaps[currentIndex], itemsWithGaps[targetIndex]] = [
   itemsWithGaps[targetIndex],
   itemsWithGaps[currentIndex],
 ];
 
+// Establish initial cumulative time from earliest anchored start (header or item)
+const firstAnchoredStart = (() => {
+  for (const it of itemsWithGaps) {
+    const t = parseTimeToSeconds(it.startTime || "");
+    if (t !== null) return t;
+  }
+  return 0;
+})();
+
 // Step 3: Recalculate item numbers and start times using stored gaps
 let itemCount = 1;
-let cumulativeEndTimeSeconds = 0;
+let cumulativeEndTimeSeconds = firstAnchoredStart;
 
 const recalculatedItems = itemsWithGaps.map((item) => {
-  if (item.type === "item") {
-    // Update item number
-    item.itemNumber = itemCount.toString();
-    itemCount++;
-
-    // Use the pre-calculated gap from before the move
-    const gap = item.calculatedGap || 0;
-    const newStartTime = cumulativeEndTimeSeconds + gap;
-
-    item.startTime = formatSecondsToTime(newStartTime);
-    cumulativeEndTimeSeconds = newStartTime;
-
-    // Add this item's duration to cumulative end time
-    const durationSeconds = parseDurationToSeconds(item.duration || "");
-    if (durationSeconds !== null) {
-      cumulativeEndTimeSeconds += durationSeconds;
-    }
-  } else if (item.type === "header") {
-    // Headers use current cumulative time but don't affect it
+  if (item.type === "header") {
     const existingStartTime = parseTimeToSeconds(item.startTime || "");
-
     if (existingStartTime !== null) {
-      // Header HAS a start time - ensure it's not before the cumulative time
-      if (existingStartTime < cumulativeEndTimeSeconds) {
+      // Advance pointer to header time if it's ahead, clamp if behind
+      if (existingStartTime > cumulativeEndTimeSeconds) {
+        cumulativeEndTimeSeconds = existingStartTime;
+      } else {
         item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds);
       }
-      // Otherwise, preserve its original time, as it's either on time or creates a gap.
     } else {
-      // Header has NO start time - set it to current cumulative time
       item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds);
     }
+    // Headers do not change cumulativeEndTimeSeconds beyond ensuring alignment
+    // Move on without incrementing itemCount
+    const { calculatedGap, ...withoutGap } = item as any;
+    return withoutGap as RunOfShowItem;
   }
 
-  // Remove the temporary calculatedGap property
-  const { calculatedGap, ...itemWithoutGap } = item;
-  void calculatedGap; // Mark as intentionally unused
-  return itemWithoutGap as RunOfShowItem;
+  // Items
+  item.itemNumber = itemCount.toString();
+  itemCount++;
+
+  const gap = item.calculatedGap || 0;
+  const newStartTime = cumulativeEndTimeSeconds + gap;
+  item.startTime = formatSecondsToTime(newStartTime);
+  cumulativeEndTimeSeconds = newStartTime;
+
+  const durationSeconds = parseDurationToSeconds(item.duration || "");
+  if (durationSeconds !== null) {
+    cumulativeEndTimeSeconds += durationSeconds;
+  }
+
+  const { calculatedGap, ...withoutGap } = item as any;
+  return withoutGap as RunOfShowItem;
 });

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly points out that hardcoding cumulativeEndTimeSeconds to 0 will break schedules that don't start at time zero. The proposed fix to initialize this value from the first item's start time is a critical correction for the feature to work as intended.

High
General
Avoid in-place mutation during mapping
Suggestion Impact:The commit stops in-place mutation inside the map by constructing new objects for both headers and items, spreading existing properties and returning copies without the temporary calculatedGap field.

code diff:

+      if (item.type === "header") {
+        const existingStartTime = parseTimeToSeconds(item.startTime || "");
+        let newHeaderStart: string;
+
+        if (existingStartTime !== null) {
+          // Advance pointer to header time if it's ahead, clamp if behind
+          if (existingStartTime > cumulativeEndTimeSeconds) {
+            cumulativeEndTimeSeconds = existingStartTime;
+            newHeaderStart = item.startTime!;
+          } else {
+            newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds);
+          }
+        } else {
+          newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds);
         }
-      } else if (item.type === "header") {
-        // Headers use current cumulative time but don't affect it
-        const existingStartTime = parseTimeToSeconds(item.startTime || "");
-
-        if (existingStartTime !== null) {
-          // Header HAS a start time - ensure it's not before the cumulative time
-          if (existingStartTime < cumulativeEndTimeSeconds) {
-            item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds);
-          }
-          // Otherwise, preserve its original time, as it's either on time or creates a gap.
-        } else {
-          // Header has NO start time - set it to current cumulative time
-          item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds);
-        }
+
+        const updatedHeader = { ...item, startTime: newHeaderStart } as RunOfShowItem;
+        const { calculatedGap, ...itemWithoutGap } = updatedHeader as any;
+        return itemWithoutGap as RunOfShowItem;
       }
 
-      // Remove the temporary calculatedGap property
-      const { calculatedGap, ...itemWithoutGap } = item;
-      void calculatedGap; // Mark as intentionally unused
+      // Items
+      const gap = (item as any).calculatedGap || 0;
+      const newStartTime = cumulativeEndTimeSeconds + gap;
+      const durationSeconds = parseDurationToSeconds(item.duration || "");
+
+      const updatedItem = {
+        ...item,
+        itemNumber: itemCount.toString(),
+        startTime: formatSecondsToTime(newStartTime),
+      } as RunOfShowItem;
+
+      itemCount++;
+      cumulativeEndTimeSeconds = newStartTime;
+      if (durationSeconds !== null) {
+        cumulativeEndTimeSeconds += durationSeconds;
+      }
+
+      const { calculatedGap, ...itemWithoutGap } = updatedItem as any;
       return itemWithoutGap as RunOfShowItem;

Avoid direct mutation of items within the .map() function during time
recalculation. Instead, create new item objects with updated properties to
ensure immutability and prevent potential React state issues.

apps/web/src/pages/RunOfShowEditor.tsx [566-602]

 // Step 3: Recalculate item numbers and start times using stored gaps
 let itemCount = 1;
 let cumulativeEndTimeSeconds = 0;
 
 const recalculatedItems = itemsWithGaps.map((item) => {
   if (item.type === "item") {
-    // Update item number
-    item.itemNumber = itemCount.toString();
+    const gap = (item as any).calculatedGap || 0;
+    const newStartTime = cumulativeEndTimeSeconds + gap;
+    const durationSeconds = parseDurationToSeconds(item.duration || "");
+
+    const updatedItem = {
+      ...item,
+      itemNumber: itemCount.toString(),
+      startTime: formatSecondsToTime(newStartTime),
+    } as RunOfShowItem;
+
     itemCount++;
-
-    // Use the pre-calculated gap from before the move
-    const gap = item.calculatedGap || 0;
-    const newStartTime = cumulativeEndTimeSeconds + gap;
-
-    item.startTime = formatSecondsToTime(newStartTime);
     cumulativeEndTimeSeconds = newStartTime;
-
-    // Add this item's duration to cumulative end time
-    const durationSeconds = parseDurationToSeconds(item.duration || "");
     if (durationSeconds !== null) {
       cumulativeEndTimeSeconds += durationSeconds;
     }
-  } else if (item.type === "header") {
-    // Headers use current cumulative time but don't affect it
+
+    const { calculatedGap, ...itemWithoutGap } = updatedItem as any;
+    return itemWithoutGap as RunOfShowItem;
+  } else {
     const existingStartTime = parseTimeToSeconds(item.startTime || "");
+    let newHeaderStart: string;
 
     if (existingStartTime !== null) {
-      // Header HAS a start time - ensure it's not before the cumulative time
       if (existingStartTime < cumulativeEndTimeSeconds) {
-        item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds);
+        newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds);
+      } else {
+        newHeaderStart = item.startTime!;
       }
-      // Otherwise, preserve its original time, as it's either on time or creates a gap.
     } else {
-      // Header has NO start time - set it to current cumulative time
-      item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds);
+      newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds);
     }
+
+    const updatedHeader = { ...item, startTime: newHeaderStart } as RunOfShowItem;
+    const { calculatedGap, ...itemWithoutGap } = updatedHeader as any;
+    return itemWithoutGap as RunOfShowItem;
   }
+});
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies direct mutation of state-derived objects within a .map() call, which violates React's immutability principles. Adopting the suggested pattern of creating new objects is a best practice that prevents potential state-related bugs.

Medium
✅ Suggestions up to commit e6315c7
CategorySuggestion                                                                                                                                    Impact
High-level
The reordering logic is flawed

The current gap preservation logic is flawed because it compares an item's old
absolute start time with the end time of its new preceding item, resulting in
incorrect timing calculations. The logic should instead calculate gaps relative
to the previous item before reordering and use that stored gap value to set
the new start time.

Examples:

apps/web/src/pages/RunOfShowEditor.tsx [536-556]
        const existingStartTime = parseTimeToSeconds(item.startTime || "");
        const expectedStartTime = cumulativeEndTimeSeconds;

        if (existingStartTime !== null) {
          // Item HAS a start time - calculate gap
          const gap = existingStartTime - expectedStartTime;

          if (gap > 0) {
            // Positive gap (intentional buffer/break) - preserve it
            item.startTime = formatSecondsToTime(expectedStartTime + gap);

 ... (clipped 11 lines)

Solution Walkthrough:

Before:

function handleMoveItem(itemId, direction) {
  // ... swap items in the array ...
  let cumulativeEndTimeSeconds = 0;

  recalculatedItems = items.map(item => {
    // This is the item's start time from its OLD position
    const existingStartTime = parseTimeToSeconds(item.startTime);
    // This is the end time of the NEW previous item
    const expectedStartTime = cumulativeEndTimeSeconds;

    // The gap is calculated incorrectly using values from different contexts
    const gap = existingStartTime - expectedStartTime;

    if (gap > 0) {
      // Preserves an incorrect gap
      cumulativeEndTimeSeconds = expectedStartTime + gap;
    } else {
      cumulativeEndTimeSeconds = expectedStartTime;
    }
    // ... update item start time and cumulative end time ...
  });
}

After:

function handleMoveItem(itemId, direction) {
  // 1. Calculate and store gaps BEFORE swapping
  const itemsWithGaps = calculateGaps(runOfShow.items);
  const itemToMove = itemsWithGaps.find(i => i.id === itemId);
  
  // 2. Swap items in the array
  // ... swap logic ...

  // 3. Recalculate times using the stored, stable gap values
  let cumulativeEndTimeSeconds = 0;
  recalculatedItems = newOrder.map(item => {
    // Use the pre-calculated gap relative to its original previous item
    const gap = item.gapBefore || 0;
    const newStartTime = cumulativeEndTimeSeconds + gap;
    item.startTime = formatSecondsToTime(newStartTime);
    
    const duration = parseDurationToSeconds(item.duration);
    cumulativeEndTimeSeconds = newStartTime + duration;
    return item;
  });
}
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical flaw in the core logic of the PR, where using an item's absolute startTime after reordering leads to incorrect gap calculations and broken timings.

High
Possible issue
Adjust header start time after reordering
Suggestion Impact:The commit added logic to check a header's existing start time against the cumulative end time and update it if earlier, matching the suggested behavior.

code diff:

@@ -565,14 +590,21 @@
         const existingStartTime = parseTimeToSeconds(item.startTime || "");
 
         if (existingStartTime !== null) {
-          // Header HAS a start time - preserve it as-is
-          // Don't update cumulative time
+          // Header HAS a start time - ensure it's not before the cumulative time
+          if (existingStartTime < cumulativeEndTimeSeconds) {
+            item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds);
+          }
+          // Otherwise, preserve its original time, as it's either on time or creates a gap.
         } else {
           // Header has NO start time - set it to current cumulative time
           item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds);
         }

When reordering, ensure a header's existing start time is not earlier than the
cumulative end time of the preceding item. If it is, update the header's start
time to match the cumulative end time.

apps/web/src/pages/RunOfShowEditor.tsx [567-573]

 if (existingStartTime !== null) {
-  // Header HAS a start time - preserve it as-is
-  // Don't update cumulative time
+  // Header HAS a start time - ensure it's not before the cumulative time
+  if (existingStartTime < cumulativeEndTimeSeconds) {
+    item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds);
+  }
+  // Otherwise, preserve its original time, as it's either on time or creates a gap.
 } else {
   // Header has NO start time - set it to current cumulative time
   item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds);
 }

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies an edge case where a header's preserved start time could become invalid after reordering, and proposes a valid fix to ensure it's at least as late as the previous item's end time. This improves the robustness of the time calculation logic.

Medium

cj-vana and others added 4 commits October 7, 2025 12:02
The previous implementation had a critical flaw: it compared an item's
old absolute start time with the end time of its new preceding item,
resulting in incorrect gap calculations.

This commit fixes the logic by:

1. Pre-calculating gaps BEFORE reordering
   - Each item's gap is calculated relative to its CURRENT previous item
   - Only positive gaps (intentional buffers) are preserved

2. Storing gap values with each item temporarily

3. After reordering, using the stored gaps to set new start times
   - New start time = previous item's end time + stored gap
   - This preserves the gap size regardless of position

4. Fixing header timing inconsistencies
   - Headers with times earlier than cumulative time are adjusted
   - Prevents misleading header timestamps after reordering

Example fix:
Before: Item with 5-min gap incorrectly calculated as 13-min gap
After: Item correctly maintains its 5-min gap after reordering

Addresses PR code review suggestions.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit addresses three critical issues in the reordering logic:

1. Fixed gap calculation to use immediate previous item
   - Previous: Incorrectly accumulated ALL previous items' times
   - Now: Walks backward to find the immediate previous item, skipping headers
   - Example fix: Item with 2-min gap was incorrectly calculated as 7-min gap

2. Preserve absolute schedule offset
   - Previous: Always started recalculation from 00:00:00
   - Now: Finds first item with a time and uses that as the base
   - Benefit: Shows starting at 20:00 (8 PM) maintain that offset after reordering

3. Implement immutable state updates
   - Previous: Mutated items directly in .map() function
   - Now: Creates new objects with updated properties
   - Benefit: Follows React best practices and prevents potential state bugs

Additional improvements:
- Early return for items without start times during gap calculation
- Better header time handling (advance cumulative time if header is ahead)
- Cleaner separation of header vs item logic in recalculation

These changes ensure gaps are correctly preserved, absolute schedule times
are maintained, and React state management follows best practices.

Addresses PR code review suggestions.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Fixed TypeScript ESLint violations:
- Removed @typescript-eslint/no-explicit-any errors by properly typing objects
- Removed @typescript-eslint/no-unused-vars errors for destructured calculatedGap

Changes:
- Instead of destructuring to remove calculatedGap, explicitly construct clean
  objects with only the needed properties
- Properly typed itemWithGap as RunOfShowItem & { calculatedGap?: number }
- Added logic to preserve custom column values while excluding calculatedGap

This maintains the same functionality while satisfying strict TypeScript rules.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Updated ImportShowFlowModal.tsx to clarify that start times should be
actual wall-clock times when items/headers occur in the show, not
sequential times starting from 00:00:00.

Changes:
- Field specification: Clarified startTime is absolute show time (e.g., "19:30:00" for 7:30 PM)
- Conversion guidelines: Updated time calculation to use actual wall-clock times
- Examples: Changed from 00:00:00/00:30:00 to realistic times (18:30:00/19:00:00)
- Added explicit note that startTime is NOT relative to show start

This fixes confusion where LLMs would generate shows with all times
starting at midnight instead of actual show times.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@cj-vana cj-vana merged commit b01ad4f into beta Oct 7, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant